Sonos binding: fix bug with notification timeout handling #3883
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree with the change, but added some comments.
@@ -11,9 +11,8 @@ | |||
</parameter> | |||
<parameter name="notificationTimeout" type="integer"> | |||
<label>Notification Timeout</label> | |||
<description>Specifies the amount of time for which the notification sound will be played</description> | |||
<description>Specifies the amount of time in seconds for which the notification sound will be played</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using the unit
attribute and the unitLabel
element?
*/ | ||
private static final Integer DEFAULT_NOTIFICATION_TIMEOUT = 40000; | ||
|
||
private static final Integer DEFAULT_NOTIFICATION_TIMEOUT = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config-description XML is using a default of 40 seconds, you change it here to 20 seconds.
I don't know what is better 20 or 40.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the XML value after your comment.
@@ -2388,7 +2388,7 @@ private void waitForFinishedNotification() { | |||
// check Sonos state events to determine the end of the notification sound | |||
String notificationTitle = stateMap.get("CurrentTitle"); | |||
long playstart = System.currentTimeMillis(); | |||
while (System.currentTimeMillis() - playstart < this.notificationTimeout.longValue()) { | |||
while (System.currentTimeMillis() - playstart < (this.notificationTimeout.longValue() * 1000)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding unneeded braces?
@@ -2407,7 +2407,7 @@ private void waitForTransportState(String state) { | |||
while (!stateMap.get("TransportState").equals(state)) { | |||
try { | |||
Thread.sleep(50); | |||
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue()) { | |||
if (System.currentTimeMillis() - start > (this.notificationTimeout.longValue() * 1000)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding unneeded braces?
@@ -2423,7 +2423,7 @@ private void waitForNotTransportState(String state) { | |||
while (stateMap.get("TransportState").equals(state)) { | |||
try { | |||
Thread.sleep(50); | |||
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue()) { | |||
if (System.currentTimeMillis() - start > (this.notificationTimeout.longValue() * 1000)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding unneeded braces?
<description>Specifies the amount of time for which the notification sound will be played</description> | ||
<default>40</default> | ||
<required>true</required> | ||
<description>Specifies the amount of time in seconds for which the notification sound will be played</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using the unit
atrribute and the unitLabel
element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I don't know this ability ;) If it is described somewhere, I will try to change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to know. I found PR #889 but no directly hint in the docs.)
EDIT: I am wrong. One can find it in the https://github.com/eclipse/smarthome/blob/091a79696626da2298968cf4ffbe82bdadab6a66/docs/documentation/development/bindings/xml-reference.md section.
@maggu2810 : I changed the config setting to include |
Please wait, there is another thing weird with PR 3808, that is the thing update. If the thing setting is not set, the binding should just use its default value without the need to update the thing setting. |
Fix bug introduced by PR #3808 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Last change done. |
Regarding the timeout value, 20s looks like a reasonable duration for notifications. Users needing very long notification will oncrease the setting. |
<default>40</default> | ||
<required>true</required> | ||
<description>Specifies the amount of time in seconds for which the notification sound will be played</description> | ||
<unitLabel>second</unitLabel> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't "s" the normal label? "5 s" looks better to me than "5 second"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be free to change it if you think it is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the code the tag unitLabel
is ignored if the attribute unit
is set (see https://github.com/eclipse/smarthome/blob/master/bundles/config/org.eclipse.smarthome.config.xml/src/main/java/org/eclipse/smarthome/config/xml/ConfigDescriptionParameterConverter.java#L120. Isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, so this line should be removed completely!
Fix bug introduced by PR #3808
Signed-off-by: Laurent Garnier lg.hc@free.fr